Skip to content

Expose active goal in stream JSON#4314

Merged
pomelo-nwu merged 3 commits into
QwenLM:mainfrom
qqqys:feat/goal-p1-protocol-coverage-clean
May 21, 2026
Merged

Expose active goal in stream JSON#4314
pomelo-nwu merged 3 commits into
QwenLM:mainfrom
qqqys:feat/goal-p1-protocol-coverage-clean

Conversation

@qqqys
Copy link
Copy Markdown
Collaborator

@qqqys qqqys commented May 19, 2026

Summary

  • emit active goal updates as stream-json partial events when partial messages are enabled
  • add the active goal stream event type to the headless message schema
  • cover headless /goal status, active status, and clear output semantics

Validation

  • cd packages/cli && npx vitest run src/nonInteractive/io/StreamJsonOutputAdapter.test.ts src/nonInteractiveCliCommands.test.ts src/ui/commands/goalCommand.test.ts src/ui/hooks/useGeminiStream.test.tsx
  • npm run build && npm run typecheck

Notes

@github-actions
Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR adds support for emitting active goal updates as stream JSON events in headless/non-interactive mode, along with improved /goal command status reporting. The implementation is clean and well-tested, with good coverage of the new functionality. The changes are focused and follow existing patterns in the codebase.

🔍 General Feedback

  • The implementation correctly follows the existing stream event pattern established for other event types (tool_progress, content deltas)
  • Test coverage is comprehensive, including edge cases like null active goal and goal clearing
  • The change properly separates concerns: type definitions, adapter logic, and command handling
  • Good attention to maintaining backward compatibility (events only emitted when includePartialMessages is enabled)
  • The goal command improvements provide consistent UX between interactive and non-interactive modes

🎯 Specific Feedback

🔴 Critical

No critical issues identified.

🟡 High

No high priority issues identified.

🟢 Medium

  • File: packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.ts:133 - Consider adding a type guard or validation for event.value before emitting. While the test shows both object and null values work correctly, an explicit check would make the code more defensive:

    if (event.type === GeminiEventType.ActiveGoal) {
      // event.value is already typed as ActiveGoal | null by ServerGeminiStreamEvent
      this.emitStreamEventIfEnabled(
        {
          type: 'active_goal',
          active_goal: event.value,
        },
        null,
      );
      return;
    }
  • File: packages/cli/src/nonInteractive/types.ts:250 - The ActiveGoalStreamEvent interface imports ActiveGoal type from core. Consider verifying this type is exported from @qwen-code/qwen-code-core's public API to avoid potential import issues in downstream consumers.

🔵 Low

  • File: packages/cli/src/ui/commands/goalCommand.ts:161 - The early return for non-interactive mode after clearing could be consolidated with the existing return pattern. Currently line 161 returns early, but line 162 falls through. Consider making the pattern consistent:

    if (context.executionMode === 'non_interactive') {
      return infoMessage(`Goal cleared: ${cleared.condition}`);
    }
    // Interactive mode: just add to history without returning message

    This is already correct, but adding a comment explaining why interactive mode returns void (to maintain existing UI behavior) would help future maintainers.

  • File: packages/cli/src/nonInteractiveCliCommands.test.ts:251 - The test "should report no active goal for empty non-interactive /goal" could benefit from also testing the interactive mode behavior for comparison, ensuring both modes handle the empty case appropriately.

  • File: packages/cli/src/nonActiveCliCommands.test.ts:274 - Consider adding a test case where /goal is called after a goal has naturally completed (achieved/failed/aborted) to verify the getLastGoalTerminal fallback behavior is exercised in non-interactive mode.

✅ Highlights

  • Test coverage excellence: The new test in StreamJsonOutputAdapter.test.ts thoroughly covers both setting and clearing active goals, including the null value case
  • Type safety: The new ActiveGoalStreamEvent interface is well-defined and integrates cleanly with the existing StreamEvent union
  • Consistent patterns: The processEvent override in StreamJsonOutputAdapter follows the exact same pattern as other stream event handlers
  • UX improvement: The non-interactive /goal command now provides actionable feedback about goal status, matching the interactive mode experience
  • Minimal diff: The changes are surgical - only touching what's needed without unnecessary refactoring

Copy link
Copy Markdown
Collaborator

@pomelo-nwu pomelo-nwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope is tight (+149/-1, focused on protocol surface + a small UX fix on top of #4273). Reading the code statically — did not run the test suite locally.

What looks good

  • Type definition is consistent. ActiveGoalStreamEvent { type: 'active_goal', active_goal: ActiveGoal | null } in types.ts mirrors the snake_case convention used by tool_progress / content_block_*, and ActiveGoal | null accurately reflects ServerGeminiActiveGoalEvent.value in core/turn.ts.
  • processEvent override reuses the existing guard. Calling emitStreamEventIfEnabled automatically respects includePartialMessages, so there is no duplicated condition. Other events go through super.processEvent(event) unchanged.
  • Nice UX side-fix in goalCommand.ts. Before this PR, non-interactive /goal clear returned void, which handleSlashCommand fell back to as "Command executed successfully." (because createNonInteractiveUI().addItem is a no-op). Returning an explicit Goal cleared: <condition> is a clear improvement.
  • Tests cover the meaningful axes. The stream-json test asserts both the populated ActiveGoal and the null (cleared) emission; the CLI tests cover empty /goal, set-then-status, and clear, and properly isolate the module-scoped store via __resetActiveGoalStoreForTests.

Suggestions

  1. Consider ACP mode in goalCommand.ts as well. The new branch is gated on executionMode === 'non_interactive', but ACP mode also goes through handleSlashCommand with the same no-op createNonInteractiveUI(), and Session.ts#processSlashCommandResult already handles messageType: 'info' by emitting an agent message chunk to Zed. So ACP users currently get the same "Command executed successfully." fallback. Loosening the check would unify the behavior:

    if (context.executionMode !== 'interactive') {
      return infoMessage(`Goal cleared: ${cleared.condition}`);
    }
  2. Optional: add a negative test for the disabled path. The "with partial messages disabled" describe block in StreamJsonOutputAdapter.test.ts doesn't assert that processEvent({ type: ActiveGoal, ... }) is suppressed. emitStreamEventIfEnabled already guards on includePartialMessages, but a single assertion would lock the behavior against future regressions.

  3. Minor architectural note (non-blocking). BaseJsonOutputAdapter exposes hooks like onTextBlockCreated / onToolUseBlockCreated and keeps state mutation in the base switch. Overriding the top-level processEvent here is fine because ActiveGoal doesn't touch message state — it's a pure side-channel event. If more side-channel events land later (e.g. hook status), it might be worth introducing an onSideChannelEvent hook in the base class for symmetry. Not needed for this PR.

Risks

  • Protocol-level: purely additive change to the stream_event discriminated union. Consumers doing exhaustive type-narrowing will need to handle the new variant, but that is the intended forward-compatible shape.
  • state.finalized guard: the base processEvent short-circuits when state.finalized is true. The override doesn't replicate that check, but per the emit sites in core/client.ts all ActiveGoal yields happen inside the turn before Finished, so this is safe today. Worth keeping in mind if the lifecycle ordering ever changes.

Overall looks good — I'd suggest folding in the ACP branch unification before merge, the rest is optional polish.

Comment thread packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.ts
Copy link
Copy Markdown
Collaborator

@wenshao wenshao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). — gpt-5.5 via Qwen Code /review

@wenshao
Copy link
Copy Markdown
Collaborator

wenshao commented May 21, 2026

Local maintainer validation — all gates green ✅

Reviewed at head 6bd59e3 (against base a7e05302) in a dedicated tmux session (pr4314, 7 windows) under git worktree /.qwen/tmp/review-pr-4314.

Environment

  • macOS 26.4.1 (Darwin 25.4.0 arm64), Node 22.17.0, npm 11.8.0
  • Clean npm ci from scratch (1453 packages installed)
  • Repo version 0.15.11

Results

Stage Command Result
Install npm ci ✅ exit 0
Build npm run build ✅ exit 0 (only pre-existing vscode-companion curly warning the PR description already calls out)
Typecheck npm run typecheck ✅ exit 0 across all 5 workspaces (acp-bridge, qwen-code, qwen-code-core, sdk, webui)
Lint npm run lint ✅ exit 0 (root eslint + integration-tests both clean)
PR-author validation suite npx vitest run StreamJsonOutputAdapter.test.ts nonInteractiveCliCommands.test.ts goalCommand.test.ts useGeminiStream.test.tsx 184/184 across 4 files
Full packages/cli suite npx vitest run 6826 passed / 9 skipped / 0 failed across 389 files (213.96s)
End-to-end against built artifact custom Node harness driving dist/.../StreamJsonOutputAdapter.js with the real GeminiEventType.ActiveGoal event emitted by packages/core/src/core/client.ts:1529 ✅ PASS

End-to-end proof (built artifact, not source)

Drove the compiled adapter with the exact event shape client.ts emits at turn start:

Set goal → expected stream event (partial=on):

{
  "type": "stream_event",
  "uuid": "0eab8d68-…",
  "session_id": "e2e-session",
  "parent_tool_use_id": null,
  "event": {
    "type": "active_goal",
    "active_goal": {
      "condition": "finish the refactor",
      "iterations": 2,
      "setAt": 123,
      "tokensAtStart": 456,
      "hookId": "goal-hook-id",
      "lastReason": "still missing verification"
    }
  }
}

Goal cleared → expected stream event (partial=on):

{
  "type": "stream_event",
  "uuid": "4c0df0b8-…",
  "session_id": "e2e-session",
  "parent_tool_use_id": null,
  "event": {
    "type": "active_goal",
    "active_goal": null
  }
}

Without --include-partial-messages (partial=off): 0 stream events emitted ✅ — gate behaves correctly.

Behavioral observations

  • processEvent override correctly bypasses the base finalized guard for ActiveGoal only — confirmed both by the new dedicated unit tests and by the e2e harness running after no startAssistantMessage() call.
  • The active_goal: null shape on goal clear is preserved end-to-end (type union in types.ts accepts ActiveGoal | null).
  • supportedModes widening to include 'acp' is reflected in the goalCommand test update and the new ACP /goal clear integration test in nonInteractiveCliCommands.test.ts.

Notes for the merge decision

  • Diff is well-scoped: +223 / -3 across 6 files (3 source + 3 test files); no surface area outside packages/cli/src/nonInteractive and packages/cli/src/ui/commands/goalCommand.ts.
  • No packages/core changes — relies on already-shipped GeminiEventType.ActiveGoal plumbing in client.ts.
  • PR is already MERGEABLE and all GitHub CI checks were green (Lint, CodeQL, Test on macOS/Ubuntu/Windows Node 22).
  • Reviewer recommendation: safe to merge. Behavior, type surface, and tests are coherent; no regressions surfaced in the 6826-test CLI suite.

— Maintainer local validation, run on 6bd59e3 from upstream pull/4314/head.

Copy link
Copy Markdown
Collaborator

@pomelo-nwu pomelo-nwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Approve

Product direction — sound. /goal drives the agent to keep iterating via a session-scoped Stop hook. Headless / stream-json / ACP consumers previously had no way to observe active-goal state (active? how many iterations? last judge reason?). Exposing ActiveGoal as an active_goal stream event, plus adding acp support to /goal, is a reasonable protocol-layer completion of the existing non-interactive goal capability (#4273).

Code quality — good.

  • The deliberate bypass of the finalized guard is necessary and correct. maybeEmitActiveGoalChange yields ActiveGoal events after the Stop hook runs (client.ts), by which point the main message is usually finalized; the base processEvent drops events once state.finalized is true. Intercepting ActiveGoal before super is exactly what lets late goal-state changes reach consumers. Since ActiveGoal never touches mainAgentMessageState, the bypass does not corrupt the message state machine. The comment explains the why well.
  • Types line up: ActiveGoalStreamEvent.active_goal: ActiveGoal | null passes event.value through directly, and the ActiveGoal fields (condition / iterations / setAt / tokensAtStart / lastReason? / hookId) match the test payload exactly.
  • Correctly silent when partial messages are disabled (covered by a test).
  • The clear-message change only returns an info message outside interactive mode, leaving the interactive history-card path intact.

Please confirm (non-blocking):

  1. ACP set->drive end-to-end: this PR adds 'acp' to supportedModes, but the set branch still does addItem(setItem) + returns submit_prompt. Tests only cover acp clear, not whether /goal <condition> under acp actually gets the host (Zed) to consume submit_prompt and kick off the first turn. A short note or an acp-set assertion would close the gap — otherwise it may "register the hook but not auto-start".
  2. Minor consistency: clear now returns a "Goal cleared: …" text receipt, but a successful set in non-interactive / acp has no "Goal set" text (it relies on the following prompt submission as implicit confirmation). A design tradeoff; fine to leave.
中文说明

评审结论:通过(Approve)

产品方向 —— 合理。 /goal 靠 session 级 Stop hook 驱动 agent 持续迭代。此前 headless / stream-json / ACP 消费者无法感知 active-goal 状态(是否激活?迭代几轮?上次判定理由?)。把 ActiveGoal 作为 active_goal stream 事件暴露,并让 /goal 支持 acp 模式,是对已有 non-interactive goal 能力(#4273)的协议层补齐,方向正确。

代码质量 —— 好。

  • 绕过 finalized guard 的设计是必要且正确的。maybeEmitActiveGoalChange 在 Stop hook 执行之后才 yield ActiveGoal 事件(client.ts),此刻主消息通常已 finalize;而 base processEventstate.finalized 为真时会丢弃事件。在 super 之前拦截 ActiveGoal,正好让晚期 goal 状态变更能送达消费者。由于 ActiveGoal 不触碰 mainAgentMessageState,绕过不会破坏消息状态机。注释把 why 写清楚了。
  • 类型吻合:ActiveGoalStreamEvent.active_goal: ActiveGoal | null 直接透传 event.valueActiveGoal 字段与测试 payload 完全一致。
  • partial messages 关闭时正确静默(有测试覆盖)。
  • clear 的文本回执只在非交互模式返回,不影响交互模式的 history card。

需作者确认(不阻塞):

  1. ACP set->驱动端到端:本 PR 把 'acp' 加进 supportedModes,但 set 分支仍是 addItem(setItem) + 返回 submit_prompt。测试只覆盖了 acp 的 clear,没覆盖 acp 下 /goal <condition> 是否真能让宿主(Zed)消费 submit_prompt 起跑首轮。补一句说明或一个 acp-set 断言即可补上 —— 否则可能"注册了 hook 但没自动开跑"。
  2. 一致性小点:clear 现在有 "Goal cleared: …" 文本回执,但 set 成功在非交互 / acp 下无 "Goal set" 文本(靠紧随的 prompt 提交隐式确认)。属设计取舍,可不改。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants